Repository navigation
Conversation
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
🤖 Reviewed all seven changed files and the supporting build/release configuration, with particular attention to artifact integrity, tag binding, credentials, subprocesses, and CI permissions. I did not identify a confirmed security vulnerability. Two release-workflow issues are noted inline.
| 7. Publish the verified draft. Alpha, beta, and RC versions are marked as prereleases automatically: | ||
|
|
||
| ```sh | ||
| gh release edit "java/v$VERSION" --repo riverqueue/river --draft=false |
There was a problem hiding this comment.
🤖 [P2] Prevent stable Java releases from replacing the repository's Latest release
For a stable version such as 0.1.0, this publish command omits --latest=false. GitHub's release-update API defaults make_latest to true for newly published releases, so publishing the Java draft can replace the Go release at the repository's /releases/latest endpoint and Latest badge. The uploader only supplies --latest=false in its prerelease creation branch, which does not protect this stable publication step. Please pass --latest=false here and in the publish command printed by upload(); covering the stable path would catch this.
| self.assertEqual( | ||
| create[:4], ["gh", "release", "create", "java/v" + self.version] | ||
| ) | ||
| for flag in ("--draft", "--verify-tag", "--prerelease", "--latest=false"): |
There was a problem hiding this comment.
🤖 [P2] Keep the prerelease fixture independent of the release version bump
The documented version-update command replaces 0.1.0-alpha.1 throughout java/, including self.version in this test module. When releasing a stable version such as 0.1.0, this test then supplies a stable manifest but still requires --prerelease and --latest=false, so the release PR fails test/java/tools. I reproduced this by applying that replacement in memory: the current ten tests pass, while the stable-version copy fails this assertion. Use a fixed synthetic prerelease fixture that the release bump will not replace (and a separate stable-version case), or explicitly exclude these release-tool fixtures from the bump.
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
🤖 I’d like to revisit making the shaded CLI JAR a required release asset. In the existing java/v0.1.0-alpha.1 release, the ten assets total 16,907,645 bytes (16.12 MiB). The river-cli-0.1.0-alpha.1-all.jar alone is 16,036,333 bytes (15.29 MiB, 94.8%); the other nine assets combined are 871,312 bytes (0.83 MiB). For scale, the ordinary river JAR is 218,795 bytes and the thin river-cli JAR is 12,821 bytes.
I inspected the published -all.jar as a ZIP. Approximately 11.20 MiB compressed is SQLite native libraries for multiple operating systems/architectures; 2.24 MiB is Jackson; 0.98 MiB is the PostgreSQL driver; River's own classes are about 0.20 MiB. This is mostly a cross-platform dependency bundle, not 15 MiB of River source. Xerial's SQLite JDBC packaging explains why its default JAR carries native binaries for many platforms and documents its without-natives and platform-specific classifier options.
My preferred change for this PR is to publish the nine POM/thin JAR/sources/Javadoc assets as the normal Maven release, and make the -all.jar an explicit, optional standalone CLI distribution rather than an entry in the required artifact_files()/manifest/upload/verification set. If we do not ship that optional distribution by default, each release's archived and mirrored bytes fall by about 95%. This fits Maven's normal model: the main JAR and POM describe the artifact and its dependencies, while an attached shaded executable is a convenience for java -jar, not a requirement for dependency consumers (Maven Shade documentation). It also keeps our release upload, verification, storage, and mirror work proportional to the actual library artifacts. At this scale I do not expect the R2 storage bill alone to be alarming; the concern is making a large duplicate bundle mandatory on every release.
There are real tradeoffs to handle explicitly:
- A thin CLI JAR cannot replace today's one-file
java -jarinstructions by itself. If that workflow is a supported product requirement, retain a separate standalone distribution or provide a dependency-resolving launcher, and update the docs and CLI smoke test accordingly. Keeping a universal standalone JAR is defensible if its convenience is worth the roughly 15 MiB per release; I would still avoid making it a prerequisite for publishing the normal Maven artifacts. - Removing
-all.jarfrom the release does not make Maven CLI downloads 15 MiB smaller: the CLI POM still declaressqlite-jdbcand PostgreSQL, and Xerial's default SQLite dependency includes the native bundle. Do not simply remove SQLite or strip the shaded JAR withminimizeJar; SQLite operation and reflectively loaded JDBC/services need to keep working. If client download size becomes important, a separate follow-up could evaluate Xerial's platform-specific native classifiers or optional driver variants, with tests on the supported platforms and both databases.
Could we make the normal release complete without -all.jar and decide separately whether the one-file CLI distribution is worth publishing?
|
Okay, getting rid of that The CLI turns out to be a bit problematic, but luckily there's reasonably easy way to manage this separately with another program called JBang. Installation becomes this: jbang app install --name river-java --java 21 \
--java-options=--enable-native-access=ALL-UNNAMED \
--repos https://riverqueue.com/maven \
--main com.riverqueue.cli.MigrationCli \
com.riverqueue:river-cli:0.1.0-alpha.1
export PATH="$HOME/.jbang/bin:$PATH"Then use is this: export DATABASE_URL=postgres://localhost/myapp
river-java migrate-list
river-java migrate-up --dry-run
river-java migrate-upYou can specify |
bgentry
left a comment
bgentry
left a comment
There was a problem hiding this comment.
Nice, LGTM other than the comments here. I couldn't test the jbang thing from a plane but probably worth making sure that'll work as expected, and then the references to internal repos should definitely not be here.
| RIVER_CLI=/path/to/river-cli-0.1.0-alpha.1-all.jar | ||
| jbang app install --name river-java --java 21 \ | ||
| --java-options=--enable-native-access=ALL-UNNAMED \ | ||
| --repos https://riverqueue.com/maven \ |
There was a problem hiding this comment.
🤖 [P1] Add Maven Central to the JBang repositories
This passes only the River repository to --repos. JBang's dependency docs say that once any repository is added, it "will no longer consult mavencentral", so it has to be listed explicitly. The River proxy serves only com.riverqueue artifacts (a request for org/postgresql/... against the dev deployment returns 404), so on a machine with an empty ~/.m2 the Postgres driver, SQLite driver, and Jackson would have nowhere to resolve from. A run on a machine that has built River before would still pass, because JBang reuses ~/.m2.
I have not been able to run this, and the docs state the rule for //REPOS rather than --repos specifically, so it is worth one test with a fresh cache (for example with JBANG_REPO pointed at an empty directory). If it fails, the fix should be:
--repos central --repos https://riverqueue.com/maven \| --directory java/target/github-release | ||
| ``` | ||
|
|
||
| To test a dev deployment, wait for its scheduled import, then add `--repository https://mod-workers-dev.riverqueue.workers.dev/maven`. For local initialization, follow the [proxy refresh instructions](https://github.com/riverqueue/gomodproxy-workers/blob/master/docs/maven.md#configuration-and-development). To verify an existing release without a local build, omit `--directory`. |
There was a problem hiding this comment.
🤖 [P1] Remove the link to a non-public repository
The "proxy refresh instructions" link points into a private repository. Public readers get a 404, and public docs should not reference non-public repositories at all. Please drop the link (and the sentence, unless the instructions can live somewhere public). The PR description has the same problem: its "Merge after" line links a pull request in that repository and should be reworded without the reference.
| --directory java/target/github-release | ||
| ``` | ||
|
|
||
| To test a dev deployment, wait for its scheduled import, then add `--repository https://mod-workers-dev.riverqueue.workers.dev/maven`. For local initialization, follow the [proxy refresh instructions](https://github.com/riverqueue/gomodproxy-workers/blob/master/docs/maven.md#configuration-and-development). To verify an existing release without a local build, omit `--directory`. |
There was a problem hiding this comment.
🤖 [P3] "Verify an existing release" does not hold for alpha.1
verify requires --version output of exactly river VERSION, but the already-published 0.1.0-alpha.1 thin JAR has no Implementation-Version and reports development, so verify --version 0.1.0-alpha.1 fails at the CLI check. Fine to leave as is, but it may be worth saying this applies to releases built from this change onward. Non-blocking.
| <filters><filter><artifact>*:*</artifact><excludes><exclude>META-INF/MANIFEST.MF</exclude><exclude>META-INF/*.SF</exclude><exclude>META-INF/*.DSA</exclude><exclude>META-INF/*.RSA</exclude><exclude>module-info.class</exclude><exclude>META-INF/versions/*/module-info.class</exclude></excludes></filter></filters> | ||
| <groupId>org.apache.maven.plugins</groupId><artifactId>maven-jar-plugin</artifactId> | ||
| <configuration><archive> | ||
| <manifest><mainClass>com.riverqueue.cli.MigrationCli</mainClass></manifest> |
There was a problem hiding this comment.
🤖 [P3] Main-Class without Class-Path
The thin JAR now advertises a main class, but java -jar river-cli.jar fails with a missing-class error because no classpath is recorded. The docs never suggest running it that way, so this is only a nit: either add <addClasspath>true</addClasspath> (with a classpathPrefix matching the dependency/ layout) or leave it. Non-blocking.
🤖 Publish Java releases through GitHub and the public River Maven proxy. Each release contains nine canonical files: the shared parent POM plus library and CLI POMs, thin JARs, sources, and Javadoc. Sonatype/signing is optional.
Remove the CLI's shaded
-all.jar. Users can installriver-javawith JBang or use Maven to download a portable directory of runtime JARs. Both paths retain Postgres and SQLite support; dependencies are cached separately instead of duplicated inside every release. Add main-class/version metadata to the thin CLI and test it with its runtime dependencies.java/bin/github-release.pyprovides three release steps:preparebuilds/tests against a temporary file repository, checks archives, and stages nine artifacts with a SHA-256 manifest.uploadrequires a clean build and matching local/remote tag, creates or resumes a draft, and verifies asset hashes. It refuses mismatches and published releases. Creation and publication use--latest=falseso Java never replaces the Go Latest release.verifychecks every proxy artifact/checksum, resolves dependencies into a fresh Maven cache, confirms repository provenance, compiles/runs a consumer, and runs thin CLI version and SQLite migration checks.Document executable release instructions for humans and local agents using
gh auth login. Java CI exercises release tooling and artifact staging. Existing published alpha.1 assets remain unchanged. Verification waits for the proxy’s scheduled import; dev/local setup links to explicit refresh instructions.Merge after https://github.com/riverqueue/gomodproxy-workers/pull/100 is deployed to production; installation instructions use
https://riverqueue.com/maven.Validation:
make check/java/github-release test/java/tools lint/java/tools: passed, including 513 Java/CLI tests, 10 Python tests, and archive checks. The nine staged artifacts total 869,774 bytes (0.83 MiB).developmentfor its version; new builds carry the version metadata.make test TEST_DATABASE=sqliteandmake lint: passed.